fix(sdk): force_flush returns meaningful bool on MetricReader#5085
fix(sdk): force_flush returns meaningful bool on MetricReader#5085ravitheja4531-cell wants to merge 9 commits intoopen-telemetry:mainfrom
Conversation
|
could someone from @jd take a look when you have a moment? |
|
I personally like this pretty much. After it's merged, we'll have to open a new issue to indicate failure reason. Will look at the code with more attention soon. Btw, are you planning to add unit tests for the new behavior? |
| **kwargs, | ||
| ) -> None: | ||
| """Called by `MetricReader.collect` when it receives a batch of metrics""" | ||
| ) -> bool: |
There was a problem hiding this comment.
| ) -> bool: | |
| ) -> bool | None: |
|
|
||
| @final | ||
| def collect(self, timeout_millis: float = 10_000) -> None: | ||
| def collect(self, timeout_millis: float = 10_000) -> Optional[bool]: |
There was a problem hiding this comment.
| def collect(self, timeout_millis: float = 10_000) -> Optional[bool]: | |
| def collect(self, timeout_millis: float = 10_000) -> bool | None: |
github-project-automation
Bot
moved this to Reviewed PRs that need fixes
in Python PR digest
| collect_ok = super().force_flush(timeout_millis=timeout_millis) | ||
| exporter_ok = self._exporter.force_flush(timeout_millis=timeout_millis) | ||
| return collect_ok and exporter_ok |
There was a problem hiding this comment.
- Should we test this logic?
- Should we even call the exporter's
force_flushif the general one fails?
There was a problem hiding this comment.
Addressed all review feedback:
Changed Optional[bool] → bool | None on collect and _receive_metrics (per @herin049)
Removed Optional from imports entirely
Fixed force_flush short-circuit: exporter.force_flush is now skipped if super().force_flush fails (per @Asquator)
Added 4 unit tests covering success, failure, short-circuit, and detach always running in finally
Ready for re-review. Thanks!
Fixes open-telemetry#5020 Signed-off-by: Ravi Theja <ravitheja4531@gmail.com>
ravitheja4531-cell
force-pushed
the
fix/force-flush-return-meaningful-bool
branch
from
60da606 to
5680779
Compare
MikeGoldsmith
left a comment
MikeGoldsmith
left a comment
There was a problem hiding this comment.
This looks good to me. Thanks @ravitheja4531-cell.
I've left a couple of small tweaks.
|
Noted changed "must" to "should" in the docstring and updated the CHANGELOG to use the PR number. Thanks! |
|
can you guys please approve it @herin049 @Asquator @MikeGoldsmith |
|
In the original code, detach(token) was called at the end of the method body but not inside a finally block. If an uncaught exception escaped the except Exception handler (e.g. a BaseException like KeyboardInterrupt), detach would never be called, leaking the context token. Moving it to finally ensures it always runs. Happy to remove the mention from the CHANGELOG if you'd prefer to keep the entry focused on the force_flush behavior. |
|
The handlng of All depends on maintainer preferences and how pedantic the library is about separating changes. Anyway, if it's left here, the merge commit message should be more generic/include both changes. |
MikeGoldsmith
left a comment
MikeGoldsmith
left a comment
There was a problem hiding this comment.
This now looks good to me, pending @herin049's feedback.
|
Hi @MikeGoldsmith @Asquator please approve so that the checks run |
|
I already approved many times. We need a maintainer to approve the workflows. |
|
You don't need to re-approve after each "update branch". A maintainer can do the final update and merge when they get to it. |
MikeGoldsmith
moved this from Reviewed PRs that need fixes
to Approved PRs
in Python PR digest
5 participants
Fixes #5020
Description
force_flushonMetricReaderandPeriodicExportingMetricReaderalwaysreturned
Trueregardless of whether the export succeeded or failed, violatingthe OTel specification:
This PR threads the actual export result up through
_receive_metrics→collect→force_flushso callers get a meaningful bool.Also fixes a pre-existing bug where
detach(token)inPeriodicExportingMetricReader._receive_metricswas only called on the happypath — moved to
finallyso it always runs.Type of change
How Has This Been Tested?
Ran the full existing metrics test suite locally: